[FIX] HW reset recovery regression - #365
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds cleanup logic before hardware reset to address a regression in HW reset recovery. The fix ensures that SERDES pipes are properly released and cached sensor values are reset before initiating a hardware reset on D4XX camera devices.
Changes:
- Added pre-reset cleanup that resets cached sensor values and releases configured SERDES pipes
- Iterates through all four sensors (depth, IR, RGB, IMU) to clear cached values and release pipes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| int release_ret = max9296_release_pipe(state->dser_dev, | ||
| sensor->pipe_id); | ||
| dev_warn(&state->client->dev, "release pipe %d (%d)\n", | ||
| sensor->pipe_id, release_ret); | ||
| sensor->pipe_configured = false; | ||
| } |
There was a problem hiding this comment.
The else clause at line 2273 is redundant. Since the if block at line 2271-2272 uses continue, the else is unnecessary and makes the code less readable. The code within the else block can be unindented to the same level as the if statement. This follows standard best practices for early returns/continues to reduce nesting.
| } else { | |
| int release_ret = max9296_release_pipe(state->dser_dev, | |
| sensor->pipe_id); | |
| dev_warn(&state->client->dev, "release pipe %d (%d)\n", | |
| sensor->pipe_id, release_ret); | |
| sensor->pipe_configured = false; | |
| } | |
| } | |
| int release_ret = max9296_release_pipe(state->dser_dev, | |
| sensor->pipe_id); | |
| dev_warn(&state->client->dev, "release pipe %d (%d)\n", | |
| sensor->pipe_id, release_ret); | |
| sensor->pipe_configured = false; |
| int release_ret = max9296_release_pipe(state->dser_dev, | ||
| sensor->pipe_id); |
There was a problem hiding this comment.
The code is calling max9296_release_pipe directly instead of using the interface abstraction pattern (state->dser_ops->release_pipe). This is inconsistent with the pattern used later in the same function at lines 2376-2377, which uses state->dser_ops->release_pipe. Direct calls bypass the interface abstraction and will fail if the deserializer is a MAX96712 instead of MAX9296. The code should use state->dser_ops->release_pipe to support both deserializer types.
| int release_ret = max9296_release_pipe(state->dser_dev, | |
| sensor->pipe_id); | |
| int release_ret = state->dser_ops->release_pipe(state->dser_dev, | |
| sensor->pipe_id); |
| if (state->dser_dev) { | ||
| mutex_lock(&serdes_lock__); | ||
| for (i = 0; i < ARRAY_SIZE(sensors); i++) { | ||
| struct ds5_sensor *sensor = sensors[i]; | ||
|
|
||
| sensor->cached_dt_value = 0xFFFF; | ||
| sensor->cached_md_value = 0xFFFF; | ||
| sensor->cached_override_value = 0xFFFF; | ||
| sensor->cached_fps_value = 0xFFFF; | ||
| sensor->cached_width_value = 0xFFFF; | ||
| sensor->cached_height_value = 0xFFFF; | ||
|
|
||
| if (!sensor->pipe_configured) { | ||
| continue; | ||
| } else { | ||
| int release_ret = max9296_release_pipe(state->dser_dev, | ||
| sensor->pipe_id); | ||
| dev_warn(&state->client->dev, "release pipe %d (%d)\n", | ||
| sensor->pipe_id, release_ret); | ||
| sensor->pipe_configured = false; | ||
| } | ||
| } | ||
| mutex_unlock(&serdes_lock__); | ||
| } |
There was a problem hiding this comment.
This new code block is not wrapped in CONFIG_VIDEO_D4XX_SERDES conditional compilation guards, unlike the similar block at lines 2357-2392. The code accesses SERDES-specific fields (state->dser_dev, state->dser_ops) and uses the serdes_lock__ mutex, which are only defined when CONFIG_VIDEO_D4XX_SERDES is enabled. This will cause compilation errors when CONFIG_VIDEO_D4XX_SERDES is not defined. Wrap this code block with #ifdef CONFIG_VIDEO_D4XX_SERDES / #endif to match the pattern used elsewhere in the function.
[FIX] HW reset recovery regression